Various fixes for Remote Attestation implementations #235
Conversation
zaolin
force-pushed
the
fix-attestation
branch
10 times, most recently
from
0bbd3af
to
e8dd2fc
Compare
zaolin
changed the title
zaolin
force-pushed
the
fix-attestation
branch
from
e8dd2fc
to
83159d3
Compare
@googlebot I fixed it. Signed-off-by: Philipp Deppenwiese <zaolin.daisuki@gmail.com>
zaolin
force-pushed
the
fix-attestation
branch
from
83159d3
to
ecde583
Compare
zaolin
assigned josephlr, twitchy-jsonp and chrisfenner and unassigned josephlr, twitchy-jsonp and chrisfenner
zaolin
changed the title
|
Hi zaolin, I apologize in advance for my slow response time as it is a busy week for me. I would be happy to review this PR and I'm targeting getting back to you by 2021-03-05. |
|
Hi @chrisfenner, no problem at all! :) |
| @@ -827,7 +845,8 @@ type CreationData struct { | |||
| OutsideInfo tpmutil.U16Bytes | |||
| } | |||
|
|
|||
| func (cd *CreationData) encode() ([]byte, error) { | |||
| // EncodeCreationData encodes byte array to TPMS_CREATION_DATA message. | |||
| func (cd *CreationData) EncodeCreationData() ([]byte, error) { | |||
There was a problem hiding this comment.
Why does this need to be exported?
(Not saying its wrong, but would like to understand the use-case. go-attestation package has been able to handle creation data by simply keeping it as bytes and only decoding when necessary, rather than doing round-trips through encode/decode).
There was a problem hiding this comment.
We use it to generate TPMS_CREATE_DATA structures for various tests.
There was a problem hiding this comment.
Also, the DecodeCreationData is already public. :)
| if _, err := rw.Write(inb); err != nil { | ||
| return nil, 0, err | ||
| } | ||
| // f(t) = (2^t)ms, up to 2s |
|
@twitchy-jsonp Any update. Does it look good to go now? |
There was a problem hiding this comment.
This change looks good to me, thank you @zaolin for all these fixes.
Unfortunately since this looks like a breaking change, we will need to strategize about how to merge. @twitchy-jsonp, @josephlr, do you think we should rev a version number?
we could introduce another flavor of the Certify and CertifyCreation commands that returns the whole signature. CertifyCreationEx and CertifyEx2? 🤢
|
I'm okay with just bumping a minor version number for this and asking users to make the (small) adjustment. |
There was a problem hiding this comment.
This change looks good to me. If @twitchy-jsonp is OK merging as-is releasing a new minor version, then I support that plan (as opposed to some solution that involves either defining new "fixed" functions)
|
I just found out about this change and I have a question. @chrisfenner Is this a "breaking change" or a "small adjustment"? Because I thought go-tpm maintains API compatibility. And in my opinion, breaking existing API is a major thing. @zaolin adding new API CreateKeyWithOutsideInfo that calls create with the new argument does not help. Because the underlying create now brakes API compatibility. |
|
Hey @tomoveu - Sorry for the inconvenience! I hope I can help. First, a note about versioning and compatibility. The breaking API change |
PolicySecret now returns three values (google/go-tpm#237) We never read the encoded value from Certify, so we are not affected by: google/go-tpm#235 Signed-off-by: Joe Richey <joerichey@google.com>
PolicySecret now returns three values (google/go-tpm#237) We never read the encoded value from Certify, so we are not affected by: google/go-tpm#235 Signed-off-by: Joe Richey <joerichey@google.com>
Summary: